Apply license choices to SPDX expressions #3425

MarcelBochtler · 2020-12-07T12:01:34Z

This is a draft how license choices should be applied.
The signature of the tested function looks like this:
fun applyChoice(choice: SpdxExpression, subtreeMatcher: SpdxExpression = this): SpdxExpression

See #3396 and #2610

spdx-utils/src/test/kotlin/SpdxExpressionTest.kt

mnonnenmacher · 2020-12-10T08:07:17Z

spdx-utils/src/main/kotlin/SpdxExpression.kt

+     * Applies a license [choice] to the given SPDX [subtreeMatcher].
+     * The [subtreeMatcher] must not contain more than two choices.
+     */
+    open fun applyChoice(subtreeMatcher: SpdxExpression, choice: SpdxExpression): SpdxExpression {


I don't understand the API here, why is choice not applied to this but to subtreeMatcher? And why must it not contain more than two choices?

why is choice not applied to this but to subtreeMatcher?

Because the choice might only be a choice of a single subtree. E.g. (a OR b) AND c.
In my implementation the user would choose b for the subtree a OR b and the result would be a AND c.
Another approach would be to not use subtree matchers at all. E.g. if the choice is b it will be chosen no matter if the expression is a OR b, a OR b OR c or (a OR b) AND c.
I had the impression from our conversation and from #2610 that this is the wanted behavior.

And why must it not contain more than two choices?

Because I thought that a OR b OR c should be handled in two separate choices:

a OR b choose b

b OR c choose b
Allowing three choices would end in even more combinations that need to be covered:

a OR b choose b

b OR c choose b

a OR b OR c choose c

If you see a use case that this separation is required I'd agree to remove this limitation.

TBH, I had a different understanding of how choices should work. In my view, a valid choice should always be checked against the whole expression. There should be no such thing as choices on sub-expressions.

That is, if the original expression would be (a OR b) AND c, then b would not be a valid choice, as it omits c. The only valid choices would be a AND c or b AND c.

Similarly, the only valid choices for a OR b OR c would be a, b or c. There should not be any "intermediate" choices like a OR b, and after applying a choice, the resulting expression must not contain OR.

@mnonnenmacher do you agree with @sschuberth?
I think this would make the API and the implementation easier and the configuration for users more straight forward.

TBH, I'm a bit surprised that making choices for sub-trees is being questioned again, as we discussed this in the meeting and I believe it became clear that it should be supported.

Picking up Sebastian's example (a OR b) AND c I believe the choice (a OR b) -> b should be applicable to it and resolve it to `b AND c´.

That is, I believe the model of a choice should be a tuple (expression, target-expression) whereas given any input expression X it would replace all subtrees of X which are equivalant to expression with target-expression.

I guess I'm simply confused about the API, even after looking at the tests, mostly due to variable naming I believe. In particular I do not like the term "subtreeMatcher", but prefer "subExpression".

I can see a value of being able to first "narrow down" the choice, and incrementally doing that to get a final choice which does not contain an OR anymore.

What would help at least me for a better understanding would be to change the function as follows:

/** * Apply a license [choice], optionally limited to the given [subExpression], and return an [SpdxExpression] where * the choice is resolved. */ open fun applyChoice(choice: SpdxExpression, subExpression: SpdxExpression = this): SpdxExpression {

And why must it not contain more than two choices?

Because I thought that a OR b OR c should be handled in two separate choices:

IMO, ideally the API would work in a way that I can generically say "whenever I come across sub-expression X, choose Y", so I should be able to say "whenever I come across sub-expression a OR b OR c, choose b" or also "whenever I come across sub-expression a OR b OR c, choose b OR c".

IMO, ideally the API would work in a way that I can generically say "whenever I come across sub-expression X, choose Y", so I should be able to say "whenever I come across sub-expression a OR b OR c, choose b" or also "whenever I come across sub-expression a OR b OR c, choose b OR c".

Fully agree, that seem 100% inline with what I envisioned as well.

I changed the API to your suggestions and tried to improve the readability.
Please also have a look at the tests that cover everything that I thought of. If a test doesn't exist I forgot that specific use case.

Short summary how the API will behave:

Whenever I come across sub-expression a OR b OR c, choose b

Whenever I come across sub-expression a OR b OR c, choose b OR c

If the sub-expression does not match the expression, the expression is returned (should we use

Throws an exception if the choice is not valid for the subExpression

@fviernau, @sschuberth please have another look at the changes

spdx-utils/src/main/kotlin/SpdxExpression.kt

spdx-utils/src/test/kotlin/SpdxExpressionTest.kt

spdx-utils/src/main/kotlin/SpdxExpression.kt

spdx-utils/src/main/kotlin/Extensions.kt

spdx-utils/src/main/kotlin/SpdxExpression.kt

sschuberth

I'm basically fine with the formal aspects of the PR now, but I'd like @mnonnenmacher to approve on the API.

spdx-utils/src/main/kotlin/SpdxExpression.kt

spdx-utils/src/test/kotlin/SpdxExpressionTest.kt

sschuberth

@fviernau @mnonnenmacher I believe this is the API we agreed on and now finally ready. Please also do a re-review so we're sure you're fine with the API and we can take it into use without expecting it to change again soon. Thanks in advance!

mnonnenmacher · 2021-01-13T11:08:59Z

spdx-utils/src/main/kotlin/SpdxExpression.kt

+    override fun applyChoice(choice: SpdxExpression, subExpression: SpdxExpression): SpdxExpression {
+        if (!subExpression.validChoices().containsAll(choice.validChoices())) {
+            throw InvalidLicenseChoiceException(
+                "$choice is not a valid choice for the subtree expression. Valid choices are: ${validChoices()}."


"subexpression" instead of "subtree expression", and I think it would be good to also print the subexpression in this message. Or maybe just say "...is not a valid choice for $subExpression. ..."

mnonnenmacher · 2021-01-13T11:11:16Z

spdx-utils/src/main/kotlin/SpdxExpression.kt

+        return replaceMatcherWithChoice(subExpression, choice)
+    }
+
+    private fun replaceMatcherWithChoice(subExpression: SpdxExpression, choice: SpdxExpression): SpdxExpression {


Is "matcher" a left-over from a renaming? Maybe rename the function to replaceSubexpressionWithChoice.

mnonnenmacher · 2021-01-13T11:12:46Z

spdx-utils/src/main/kotlin/SpdxExpression.kt

+
+    private fun replaceMatcherWithChoice(subExpression: SpdxExpression, choice: SpdxExpression): SpdxExpression {
+        val expressionString = toString()
+        val matcherString = subExpression.toString()


As above, rename to subexpressionString?

mnonnenmacher · 2021-01-13T11:13:58Z

spdx-utils/src/main/kotlin/SpdxExpression.kt

@@ -516,3 +558,5 @@ enum class SpdxOperator(
     */
    OR(0)
 }
+
+class InvalidLicenseChoiceException(message: String) : Exception(message)


Would it make sense to extend SpdxException instead?

mnonnenmacher · 2021-01-13T11:19:11Z

spdx-utils/src/test/kotlin/SpdxExpressionTest.kt

+                val expression = "a OR b OR c".toSpdx()
+                val choice = "a".toSpdx()
+
+                val result = expression.applyChoice(choice)


Currently this is the same test as "apply the choice if the expression contains multiple choices" below, according to the test name I think you wanted to pass a subexpression here.

It was a leftover from before the API change where not more than one choice were allowed. As now choices can be applied I removed this test.

mnonnenmacher · 2021-01-13T11:20:15Z

spdx-utils/src/test/kotlin/SpdxExpressionTest.kt

+                result shouldBe "b".toSpdx()
+            }
+
+            "throw an exception if the chosen license is not an valid option" {


Typo: "a valid"

mnonnenmacher · 2021-01-13T11:28:15Z

spdx-utils/src/test/kotlin/SpdxExpressionTest.kt

+                val choice = "x".toSpdx()
+                val subExpression = "x OR y OR z".toSpdx()
+
+                val result = expression.applyChoice(choice, subExpression)


Shouldn't it throw an exception if the subexpression is invalid like for invalid license choices?

Yes I agree that this would make the API more consistent and will fail early if the developer uses this API in another way as it was intended.

mnonnenmacher · 2021-01-13T11:31:17Z

@fviernau @mnonnenmacher I believe this is the API we agreed on and now finally ready. Please also do a re-review so we're sure you're fine with the API and we can take it into use without expecting it to change again soon. Thanks in advance!

I'm mostly fine with the API and had only minor comments. The only API change I mentioned in my comments is if we should throw an exception if applyChoice is called with an invalid subExpression like we do for invalid choices.

To be able to apply license choices to SpdxCompoundExpressions, a logic is added to apply the user's choices to a specified matcher. Signed-off-by: Marcel Bochtler <marcel.bochtler@bosch.io>

sschuberth

Also need @mnonnenmacher to re-review and approve.

sschuberth · 2021-01-13T14:26:08Z

Merging this despite the stuck markdown-link-check as all other checks passed.

MarcelBochtler requested review from fviernau, mnonnenmacher and sschuberth as code owners December 7, 2020 12:01

MarcelBochtler marked this pull request as draft December 7, 2020 12:01

MarcelBochtler commented Dec 7, 2020

View reviewed changes

spdx-utils/src/test/kotlin/SpdxExpressionTest.kt Outdated Show resolved Hide resolved

MarcelBochtler force-pushed the license-choice branch 6 times, most recently from 143e47f to 93b2c38 Compare December 9, 2020 14:25

MarcelBochtler changed the title ~~Proposal: Apply license choices~~ Apply license choices to SPDX expressions Dec 9, 2020

MarcelBochtler force-pushed the license-choice branch from 93b2c38 to 7f63032 Compare December 9, 2020 14:29

MarcelBochtler marked this pull request as ready for review December 9, 2020 14:29

MarcelBochtler requested a review from tsteenbe as a code owner December 9, 2020 14:29

MarcelBochtler force-pushed the license-choice branch 2 times, most recently from e590394 to 7e24729 Compare December 10, 2020 08:00

mnonnenmacher requested changes Dec 10, 2020

View reviewed changes

sschuberth requested changes Dec 10, 2020

View reviewed changes

MarcelBochtler force-pushed the license-choice branch from 7e24729 to c0a5aaf Compare December 11, 2020 10:08

MarcelBochtler requested review from sschuberth and mnonnenmacher December 11, 2020 10:08

sschuberth requested changes Dec 11, 2020

View reviewed changes

MarcelBochtler force-pushed the license-choice branch from c0a5aaf to ad5a0fc Compare December 11, 2020 11:41

MarcelBochtler requested a review from sschuberth December 11, 2020 11:41

sschuberth reviewed Dec 11, 2020

View reviewed changes

spdx-utils/src/main/kotlin/SpdxExpression.kt Outdated Show resolved Hide resolved

MarcelBochtler force-pushed the license-choice branch from ad5a0fc to 809f0b5 Compare December 12, 2020 23:46

MarcelBochtler requested a review from sschuberth December 13, 2020 00:10

MarcelBochtler force-pushed the license-choice branch 2 times, most recently from d7394ec to ff77dae Compare December 14, 2020 16:35

MarcelBochtler force-pushed the license-choice branch 5 times, most recently from 773604a to c3dc5de Compare January 12, 2021 14:37

sschuberth requested changes Jan 12, 2021

View reviewed changes

spdx-utils/src/test/kotlin/SpdxExpressionTest.kt Outdated Show resolved Hide resolved

MarcelBochtler force-pushed the license-choice branch from c3dc5de to 5bdafe5 Compare January 12, 2021 14:45

MarcelBochtler requested a review from sschuberth January 12, 2021 14:47

MarcelBochtler force-pushed the license-choice branch 2 times, most recently from 5cfbac4 to 2d61fa4 Compare January 12, 2021 15:03

sschuberth previously approved these changes Jan 12, 2021

View reviewed changes

mnonnenmacher requested changes Jan 13, 2021

View reviewed changes

MarcelBochtler dismissed sschuberth’s stale review via 4bcfa51 January 13, 2021 12:23

MarcelBochtler force-pushed the license-choice branch from 2d61fa4 to 4bcfa51 Compare January 13, 2021 12:23

MarcelBochtler requested review from mnonnenmacher and sschuberth January 13, 2021 12:24

SpdxExpression: Add method to apply a choice on a SPDX expression

fe23cd3

To be able to apply license choices to SpdxCompoundExpressions, a logic is added to apply the user's choices to a specified matcher. Signed-off-by: Marcel Bochtler <marcel.bochtler@bosch.io>

MarcelBochtler force-pushed the license-choice branch from 4bcfa51 to fe23cd3 Compare January 13, 2021 12:27

sschuberth approved these changes Jan 13, 2021

View reviewed changes

mnonnenmacher approved these changes Jan 13, 2021

View reviewed changes

sschuberth merged commit b65017c into oss-review-toolkit:master Jan 13, 2021

sschuberth deleted the license-choice branch January 13, 2021 14:26

sschuberth mentioned this pull request Feb 9, 2021

Proposal: Implement license choices #2610

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply license choices to SPDX expressions #3425

Apply license choices to SPDX expressions #3425

MarcelBochtler commented Dec 7, 2020 •

edited

Loading

mnonnenmacher Dec 10, 2020

MarcelBochtler Dec 10, 2020

sschuberth Dec 15, 2020 •

edited

Loading

MarcelBochtler Dec 17, 2020

fviernau Dec 17, 2020 •

edited

Loading

sschuberth Dec 17, 2020 •

edited

Loading

sschuberth Dec 17, 2020

fviernau Dec 17, 2020

MarcelBochtler Jan 8, 2021

MarcelBochtler Jan 11, 2021

sschuberth left a comment

sschuberth left a comment

mnonnenmacher Jan 13, 2021

mnonnenmacher Jan 13, 2021

mnonnenmacher Jan 13, 2021

mnonnenmacher Jan 13, 2021

mnonnenmacher Jan 13, 2021

MarcelBochtler Jan 13, 2021

mnonnenmacher Jan 13, 2021

mnonnenmacher Jan 13, 2021

MarcelBochtler Jan 13, 2021

mnonnenmacher commented Jan 13, 2021

sschuberth left a comment

sschuberth commented Jan 13, 2021

Apply license choices to SPDX expressions #3425

Apply license choices to SPDX expressions #3425

Conversation

MarcelBochtler commented Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

sschuberth Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth left a comment

Choose a reason for hiding this comment

sschuberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnonnenmacher commented Jan 13, 2021

sschuberth left a comment

Choose a reason for hiding this comment

sschuberth commented Jan 13, 2021

MarcelBochtler commented Dec 7, 2020 •

edited

Loading

sschuberth Dec 15, 2020 •

edited

Loading

fviernau Dec 17, 2020 •

edited

Loading

sschuberth Dec 17, 2020 •

edited

Loading